Skip to content

Conversation

FelixVaughan
Copy link
Contributor

This relates to...

#4444

Rationale

Preserve header shape and correct Host after DNS resolution to avoid routing issues/header corruption

Changes

Added addHostHeader to dns interceptor and parameterized tests for different header types.

Bug Fixes

Fixed DNS interceptor turning [string, string][]/iterables into numeric-key headers

Status

@@ -5,6 +5,43 @@ const DecoratorHandler = require('../handler/decorator-handler')
const { InvalidArgumentError, InformationalError } = require('../core/errors')
const maxInt = Math.pow(2, 31) - 1

function addHostHeader (headers, host) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you don't need to parse the headers by yourself, this is done by undici automatically, we just need to standardise the way that these are forward to undici.

If Array, push host to the headers array, if an object, attach host as a new property

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've simplified the code!

A header case I'm slightly worried about though is non-array iterables.

request({ origin, path: '/', method: 'GET', headers: new Set([['f0', 'b0'], ['f1', 'b1']]) }).

These headers are valid under UndiciHeaders and a request like this (though rare) is possible; but the proposed array/object check would break this.

Another issue I've noticed is [string,string][] headers always raise an error due to the way header processing is handled in Request.js. Say headers = [['f0', 'b0']] or [['f0', 'b0'], ['f1', 'b1']]

if (Array.isArray(headers)) {
      if (headers.length % 2 !== 0) {
        throw new InvalidArgumentError('headers array must be even')
      }
      for (let i = 0; i < headers.length; i += 2) {
        processHeader(this, headers[i], headers[i + 1])
      }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants